Conversation
wilsonjord
left a comment
wilsonjord
left a comment
There was a problem hiding this comment.
I suspect it would be more complicated, but since []string is being passed to SendBatchN, perhaps instead of returning a slice of failed bodies, a slice of failed indexes could done? Returning body copies seems inefficient.
| type SendBatchError struct { | ||
| err error | ||
| info []SendBatchErrorInfo | ||
| } | ||
| type SendBatchErrorInfo struct { | ||
| entry types.BatchResultErrorEntry | ||
| body string | ||
| } |
There was a problem hiding this comment.
issue: error handling not quite as clean as it could be; seems to me that I need to check the Info field for empty, before knowing how to handle the error, which seems a little backwards. I also don't know why the fields aren't simply exported. Would you consider wrapping the general err, and separating it out from "partial message failure"? Something like:
var SendBatchPartialError = errors.New("partial message failure")
type SendBatchError struct {
Err error
Info []SendBatchResultErrorEntry
}
func (e *SendBatchError) Unwrap() error {
return e.Err
}
void main() {
err := sqs.SendBatch(payload)
if errors.Is(err, SendBatchPartialError) {
// deal with err.Info
} else if err != nil {
// general error, I can should be able to access wrapped error if desired
}
}
aws/sqs/sqs.go
Outdated
| for i := range bodies { | ||
| // Check if any single message is too big | ||
| if len(bodies[i]) > maxSize { | ||
| sendBatch(i) |
There was a problem hiding this comment.
question: why this if we know message is too big?
CallumNZ
force-pushed
the
batchMethods
branch
2 times, most recently
from
b9c89a3 to
ea86180
Compare
CallumNZ
force-pushed
the
batchMethods
branch
2 times, most recently
from
3495361 to
2935362
Compare
This is a shared library
|
@wilsonjord an example of using it: https://github.com/GeoNet/tilde/blob/b4cd55a4a5dd930bcfe78cc6034de4a27ff7788c/cmd/tilde-router/sqs-batcher.go#L59 The current idea is: If there's an HTTP error, all elements of batch are added to So, if there's any sort of error, the user will be looping over |
CallumNZ
force-pushed
the
batchMethods
branch
2 times, most recently
from
f19f503 to
5a5f648
Compare
Added to test that test delete function
Includes adding test for this method
CallumNZ
force-pushed
the
batchMethods
branch
2 times, most recently
from
71d7857 to
c9f0d5a
Compare
Includes adding a test for this method BREAKING CHANGE: now returns the number of API calls to SendBatch made
Changes the underlying receiveMethod function to return all messages rather than just the first
4 participants
Changes proposed in this pull request:
Production Changes
The following production changes are required to deploy these changes:
Review
Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer.
When assigning the code review please consider the expertise needed to review the changes.
Code Review Guide
Insert check list here if needed.